Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gate if-let guard feature #74566

Merged
merged 3 commits into from
Aug 22, 2020
Merged

Gate if-let guard feature #74566

merged 3 commits into from
Aug 22, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jul 20, 2020

Enhanced on #74315. That PR is in crater queue so I don't want to push to it.

Close #74232
cc #51114

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2020
@tesuji

This comment has been minimized.

@estebank
Copy link
Contributor

tidy error: Found 1 features without a gate test.
Expected a gate test for the feature 'if_let_guard'.
Hint: create a failing test file named 'feature-gate-if_let_guard.rs'
      in the 'ui' test suite, with its failures due to
      missing usage of `#![feature(if_let_guard)]`.
Hint: If you already have such a test and don't want to rename it,
      you can also add a // gate-test-if_let_guard line to the test file.

@tesuji tesuji force-pushed the guard branch 2 times, most recently from a7c36d6 to 4810d29 Compare July 21, 2020 05:04
@estebank
Copy link
Contributor

LGTM, letting @eddyb have final r+.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 24, 2020

Crater result in #74315 : #74315 (comment)

There are real regressions here.

This snippet now fails to compile when previously accepted:

#[derive(PartialEq)]
struct Foo {
    a: i32,
    b: f32,
}

let f = Foo { a: 42, b: 0.0 };
match () {
    () if f == Foo { a: 0, b: 0.0 } => {}
    //~^ ERROR: struct literals are not allowed here
    _ => {}
}

But note that the if expression has the same restrictions without this PR:

if f == Foo { a: 0, b: 0.0 } {
//~^ ERROR: struct literals are not allowed here
}
error: struct literals are not allowed here
  --> src/lib.rs:14:13
   |
14 |     if f == Foo { a: 0, b: 0.0 } {
   |             ^^^^^^^^^^^^^^^^^^^^
   |
help: surround the struct literal with parentheses
   |
14 |     if f == (Foo { a: 0, b: 0.0 }) {
   |             ^                    ^

error: aborting due to previous error

I guess we have to accept this fate and fallback to previous parse_expr.

@tesuji tesuji force-pushed the guard branch 2 times, most recently from 1d29eb1 to df96b4f Compare July 24, 2020 16:30
@tesuji
Copy link
Contributor Author

tesuji commented Jul 24, 2020

Updated.

@estebank
Copy link
Contributor

I guess we have to accept this fate and fallback to previous parse_expr.

Shame. We could maybe change that in 2021?

@tesuji
Copy link
Contributor Author

tesuji commented Jul 25, 2020

There are 12 regressed crates out of 113052 crates tested.

Spurious:

  • 2 is linker error:

    • artichoke/artichoke/1bd7af559015500752ae84f90df715cfbb8f6a09
    • tracing-attributes-0.1.9
  • Network error:

    • corysabol.HyperFlo.cb68cc080eacf87a0f0737ece7c195dae1dd98ce
    • divy-work.deno-notify.f82302fc180ea17665d6b47b8de6666bab0aeb0b

Real regressions:


There are breakages when disallowing struct literals in match guard.
The breakages is small and easy to fix
(I will fix them upstream if we agree on this move).
But disallowing struct literals on match guard
match restrictions on if condition and makes error reporting/parsing easier.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 25, 2020

This is Underspecified language semantics.

On reference, match arm guard is if expression, but if expr doesn't allow struct expression.

Can I nominate this issue for T-lang or T-compiler?
Join the discussion on Zulip: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Underspecified.20language.20semantics.20of.20match.20arm.20guard

@alercah
Copy link
Contributor

alercah commented Jul 25, 2020

The reason unwrapped struct expressions aren't allowed in if is due to ambiguity of {. There's no such ambiguity in a guard.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 26, 2020

The reason unwrapped struct expressions aren't allowed in if is due to ambiguity of {. There's no such ambiguity in a guard.

Yes, I agree there are no ambiguity in the correct code.
But with the incorrect code, parser has to recover and point out erroring span.
Let's take a look at this file to see the diagnostic: https://github.com/rust-lang/rust/blob/master/src/test/ui/parser/issue-15980.rs
It is not bad but not good either.

@bors

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Aug 9, 2020

This doesn't seem like something that should be changed in a PR - this is a breaking change to the language itself. Can we try to improve the diagnostic instead?

@tesuji
Copy link
Contributor Author

tesuji commented Aug 9, 2020

I don't change that in this PR. I will open a new issue for discussing it.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 12, 2020

Can we merge this PR, @eddyb ? This PR in current state doesn't break old code.

@jyn514
Copy link
Member

jyn514 commented Aug 12, 2020

You may want to find a different reviewer, I know that eddyb is very behind on github notifications 😅

@tesuji
Copy link
Contributor Author

tesuji commented Aug 12, 2020

Thanks. I don't know what appropriate reviewers. I would wait a few days before finding another one.

@tesuji tesuji closed this Aug 12, 2020
@tesuji tesuji reopened this Aug 12, 2020
@eddyb
Copy link
Member

eddyb commented Aug 16, 2020

This seems to be a parsing change, so:
r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned eddyb Aug 16, 2020
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2020

📌 Commit ef50477 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2020
@bors
Copy link
Contributor

bors commented Aug 22, 2020

⌛ Testing commit ef50477 with merge 5528caf...

@bors
Copy link
Contributor

bors commented Aug 22, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 5528caf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2020
@bors bors merged commit 5528caf into rust-lang:master Aug 22, 2020
@bors bors mentioned this pull request Aug 22, 2020
@tesuji tesuji deleted the guard branch August 23, 2020 01:57
@rust-log-analyzer
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
##[section]Starting: Request a runner to run this job
Found online and idle self-hosted runner in current repository that matches the required labels: 'self-hosted , ARM64 , linux'

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if-let guard point to let_chains issue
10 participants